Allowing no scopes to be provided by wmathurin · Pull Request #2777 · forcedotcom/SalesforceMobileSDK-Android

wmathurin · 2025-10-02T02:56:01Z

Checking the scope in the token endpoint response to make sure we got refresh_token
Refactored onAuthFlowComplete() to make it testable and added tests for it

Checking the scope in the token endpoint response to make sure we got refresh_token (and id)

Throwing an error if it isn't. Refactored onAuthFlowComplete() to make it testable and added tests for it.

github-actions · 2025-10-02T02:56:29Z

	1 Warning
⚠️	Big PR, try to keep changes smaller if you can.

Generated by 🚫 Danger

wmathurin · 2025-10-02T02:58:30Z

+}
+
+@VisibleForTesting
+internal suspend fun onAuthFlowComplete(


This is like the original onAuthFlowComplete except with pieces refactored out in private helper methods - which are not called directly but are called through lambda parameters.

wmathurin · 2025-10-02T02:59:19Z

        .clientId(consumerKey)
        .nativeLogin(nativeLogin)
        .build()
-    account.downloadProfilePhoto()


Moved this one into fun addAccount(account: UserAccount?, context: Context, isTestRun: Boolean, loginServerManager: LoginServerManager)

wmathurin · 2025-10-02T03:00:02Z

+    val scopeParser = ScopeParser(tokenResponse.scope)
+
+    // Check that it has the refresh token scope, otherwise call onAuthFlowError
+    if (!scopeParser.hasRefreshTokenScope()) {


This check is new to this PR.

Is not having the refresh token scope actually an invalid state/error? Do you still get the refresh scope if the policy is set to expire immediately?

We never allowed it. We would add the refresh_token scope during login if it was not in bootconfig. So login would fail if the connected app did not have it configured.
I need to try and see what happens if the connected app does not have the scope and I don't check it and also what happens if it is set to expire immediately.

@brandonpage
I just tried the following:

an ECA that does not have refresh_token and a client app that does not specify scopes in bootconfig (and without the check above) => login works / token end point response does not include a refresh token

an ECA with refresh token set to expire immediately => login works / token end point response includes a refresh token (but it will fail to refresh the access token when used)

I'm okay removing the check. We should include those scenarios in our test plan. Especially to make sure that having that null refresh token does not lead to some unwanted behaviors.

wmathurin · 2025-10-02T03:00:38Z

+    }
+
+    @Test
+    fun testOnAuthFlowComplete_blockIntegrationUser_shouldCallError() = runTest {


Tests against the refactored onAuthFlowComplete().

wmathurin · 2025-10-02T03:01:52Z

-     * @return Scope parameter.
+     * @return Scope parameter string (possibly empty).
     */
    public static String computeScopeParameter(String[] scopes) {


This is an important change for this PR.
ScopeParser.computeScopeParameter(scopes) will leave an empty scopes list unchanged.

wmathurin · 2025-10-02T03:02:09Z

+         * @return Scope parameter string (possibly empty).
+         */
+        @JvmStatic
+        fun computeScopeParameter(scopes: Array<String>?): String {


This is the critical change of this PR.

brandonpage

Looks good! Really appreciate all of the tests and the effort put into making onAuthFlowComplete more testable!

brandonpage · 2025-10-02T17:44:16Z

-			}
-		}
-		return false;
+		return new ScopeParser(scope).hasScope(scopeToCheck);


~~The syntax isn't as clean in Java, but I wonder if ScopeParser could just be a couple String[] extensions?~~

Edit: There are several functions in ScopeParser so I see its need to be a class.

brandonpage · 2025-10-03T01:58:42Z

+    onAuthFlowComplete(
+        tokenResponse,
+        loginServer,
+        consumerKey,
+        onAuthFlowError,
+        onAuthFlowSuccess,
+        buildAccountName,
+        nativeLogin,
+        SalesforceSDKManager.getInstance().appContext,
+        SalesforceSDKManager.getInstance().userAccountManager,
+        blockIntegrationUser,
+        getRuntimeConfig(SalesforceSDKManager.getInstance().appContext),
+        ::updateLoggingPrefsHelper,
+        ::fetchUserIdentity,
+        ::startMainActivityHelper,
+        ::setAdministratorPreferences,
+        ::addAccountHelper,
+        ::handleScreenLockPolicy,
+        ::handleBiometricAuthPolicy,
+        ::handleDuplicateUserAccount
+    )


Since the original onAuthFlowComplete is internal you could just update the original and add all of the new values with these as the default parameters.

brandonpage · 2025-10-03T02:15:26Z

+    val scopeParser = ScopeParser(tokenResponse.scope)
+
+    // Check that it has the refresh token scope, otherwise call onAuthFlowError
+    if (!scopeParser.hasRefreshTokenScope()) {


Is not having the refresh token scope actually an invalid state/error? Do you still get the refresh scope if the policy is set to expire immediately?

brandonpage · 2025-10-03T02:44:07Z

+    SalesforceSDKManager.getInstance().appContext.startActivity(Intent(SalesforceSDKManager.getInstance().appContext, SalesforceSDKManager.getInstance().mainActivityClass).apply {
+        setPackage(SalesforceSDKManager.getInstance().appContext.packageName)
+        flags = FLAG_ACTIVITY_NEW_TASK
+    })


NIT: Why remove the with(SalesforceSDKManager.getInstance()) block?

I can put it back - it was more readable.

brandonpage · 2025-10-03T03:02:52Z

+        const val REFRESH_TOKEN = "refresh_token"
+        const val ID = "id"
+        private const val SINGLE_SPACE = " "
+        private const val EMPTY_STRING = ""


I don't dislike this, but I think we typically just use "" in code like how 0 isn't really a magic number. 🤷

Agreed, I will get rid of it.

A quick test indicated that: - login with a CA/ECA that does not have refresh scope works but the token end point does not contain a refresh token - login with a CA/ECA that has a refresh token that expires immediately works also More testing is needed to make sure that expired or revoked access token is gracefully handled when we don't have a refresh token (and also when we have an expired refresh token).

wmathurin · 2025-10-03T22:11:31Z

The only test failure "LoginActivityTest.viewModelIsUsingFrontDoorBridge_DefaultValue_onCreateWithoutQrCodeLoginIntent" is unrelated.

wmathurin added 2 commits September 30, 2025 21:39

Allowing no scopes to be provided

60ee59a

Checking the scope in the token endpoint response to make sure we got refresh_token (and id)

Checking that refresh scope came back in token end point response

9f0d652

Throwing an error if it isn't. Refactored onAuthFlowComplete() to make it testable and added tests for it.

wmathurin commented Oct 2, 2025

View reviewed changes

brandonpage approved these changes Oct 3, 2025

View reviewed changes

wmathurin added 2 commits October 3, 2025 11:39

Addressing feedback

e4793b3

wmathurin merged commit 7d868cd into forcedotcom:dev Oct 3, 2025
4 of 6 checks passed

Uh oh!

Conversation

wmathurin commented Oct 2, 2025

Uh oh!

github-actions Bot commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonpage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmathurin commented Oct 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants